-
Notifications
You must be signed in to change notification settings - Fork 115
add Mint.HTTP.module/1 to describe the conn module given a conn #324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Would it be better to expose the protocol instead of the module? Something like |
|
In the scenario from #263 I think it's better to return the connection module since they want to check if they can call functions on a specific module. There may be a use case to get the protocol as well but that would need a different API I think since we support HTTP 0.9, 1.0, 1.1, and 2.0. Maybe |
|
@ericmj yes I like the protocol + version tuple. Can't folks map the protocol to the HTTP module? Wondering if we should keep the module internal, that's all :) |
|
I don't think we need to keep it internal, We can add |
|
I’m worried about the proxy too here. If we return the module it's not guaranteed that you can call functions on that, is it? Because it could be a proxy conn? |
|
For our purposes, we're looking to differentiate in order to fix a bug with |
Yeah, this would break on a proxy conn. @andyleclair Sorry for the back and forth, but could you add a |
|
hey no problem! I'm glad to help. thank you both for your time (and for considering this PR) |
|
From my understanding, I'd need a request to determine the full protocol from the conn, right? Obviously for HTTP2, it's just |
Ugh, good point. In that case I am leaning towards returning |
|
Under the hood |
|
I would just go with |
|
@ericmj @whatyouhide how does that look to you? |
|
Looks good but I think we should handle UnsafeProxy or did we decide not to? |
|
@ericmj since |
whatyouhide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions, plus I’m requesting changes because we need to add tests for this 🙃
lib/mint/http.ex
Outdated
| ## Examples | ||
| :http1 = Mint.HTTP.protocol(conn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use this format:
| :http1 = Mint.HTTP.protocol(conn) | |
| Mint.HTTP.protocol(conn) | |
| #=> :http1 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added doctests, is that ok?
|
@whatyouhide it looks like using |
|
@andyleclair you can do if Version.compare(System.version(), "<whatever version introduced @doc since>") in [:eq, :gt] do
@doc since: "1.4.0"
end |
@ericmj it seems like that could also be achieved by making
Mint.HTTP.conn_modulepublic, but I'm not sure aboutUnsafeProxy. Since it usesMint.HTTP1under the hood should we returnMint.HTTP1?UnsafeProxy? I don't really know whatUnsafeProxygets used for, guessing non-https proxying